Add support for external class validators #67
Conversation
|
@DamianEdwards would this be something you would consider merging. We found this much easier to convert our existing fluent validation while leaving our models clean. |
|
@DamianEdwards now that .NET 9 has shipped, would this be something you would accept? |
src/MiniValidation/MiniValidator.cs
Outdated
| @@ -532,6 +535,47 @@ private static async Task<bool> TryValidateImpl( | |||
| } | |||
| } | |||
|
|
|||
| if (isValid) | |||
| { | |||
| var validators = (IEnumerable?)serviceProvider?.GetService(typeof(IEnumerable<>).MakeGenericType(typeof(IValidatable<>).MakeGenericType(targetType))); | |||
There was a problem hiding this comment.
It'd be better to cache as much of this reflection in the type cache while walking the type. We don't want to be calling MakeGenericType on every validate call if we can avoid it. The type cache could probably just do this discovery of validators from IServiceProvider up front and make those types available. Also you can check if the IServiceProvider supports IServiceProviderIsService and use that to check for validators rather than performing a lookup and checking for null (which can have side-effects), but if we move all this into the type cache that's probably not important anymore.
There was a problem hiding this comment.
Cached and used IServiceProviderIsService
src/MiniValidation/MiniValidator.cs
Outdated
| var validatorMethod = validator.GetType().GetMethod(nameof(IValidatable<object>.ValidateAsync)); | ||
| if (validatorMethod is null) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"The type {validators.GetType().Name} does not implement the required method 'Task<IEnumerable<ValidationResult>> ValidateAsync(object, ValidationContext)'."); | ||
| } | ||
|
|
||
| var validateTask = (Task<IEnumerable<ValidationResult>>?)validatorMethod.Invoke(validator, | ||
| new[] { target, validationContext }); |
There was a problem hiding this comment.
Invoking this via reflection every time is not ideal. Would be good to figure out if there's a way to avoid this somehow but, e.g. generating a delegate in the type cache that can be invoked from here without the generic (the body of the generated method would just cast to the target type).
Pre-generating a delegate would mean we could avoid the checks here too on every call and just store the exception up front when the type cache is being built and generate the method to just throw.
There was a problem hiding this comment.
Added as much caching as I could. I couldn't figure out how to get MethodInfo.CreateDelegate to work because we need to cast the target parameter to the target type, but at runtime, we don't know that type.
There was a problem hiding this comment.
Ok, figured out how to use expressions to compile a delegate that casts the parameter.
src/MiniValidation/IValidatable.cs
Outdated
| /// Provides a way to add a validator for a type outside the class. | ||
| /// </summary> | ||
| /// <typeparam name="T"></typeparam> | ||
| public interface IValidatable<in T> |
There was a problem hiding this comment.
Not 100% sold on the name. Some alternatives to consider:
IValidate<TTarget>IValidator<TTarget>IValidatorFor<TTarget>
src/MiniValidation/IValidatable.cs
Outdated
| /// <param name="instance">The object instance to validate.</param> | ||
| /// <param name="validationContext">The validation context.</param> | ||
| /// <returns>A collection that holds failed-validation information.</returns> | ||
| Task<IEnumerable<ValidationResult>> ValidateAsync(T instance, ValidationContext validationContext); |
There was a problem hiding this comment.
Should we have a non-async version? Or do we think given this is a net-new addition it's fine to just make it async-only? Do the non-async validate methods throw if an IValidatable is registered on the type you're trying to validate?
There was a problem hiding this comment.
It feels like this whole library could be simplified if it was async / valuetask only, but that would obviously be a breaking change and dataannotations doesn't support async either. I really wish .NET would modernize data annotations and make them more powerful. :-)
Anyway, it would be pretty trivial to add non-async support. Up to you.
There was a problem hiding this comment.
I think for net-new stuff it's fine to say it's async only. This needs to add logic to throw if the non-async validate method is called on a type that has one of these registered.
There was a problem hiding this comment.
It throws for non-async already using the ThrowIfAsyncNotAllowed method.
|
@ejsmith @niemyjski thanks for this. I quite like the idea but have some suggested implementation changes and open questions. |
|
@DamianEdwards I believe I addressed your feedback. Let me know what you think. |
|
@DamianEdwards I ended going a bit crazy and doing more changes in this PR. I probably should've broken the changes out. I added service provider extension methods to make working with MiniValidation easier for DI and I added a non-async Let me know what you think. |
|
|
||
| if (TryGetAttributesViaTypeDescriptor(property, out var typeDescriptorAttributes)) | ||
| if (customAttributes.Count == 0 && TryGetAttributesViaTypeDescriptor(property, out var typeDescriptorAttributes)) |
There was a problem hiding this comment.
I noticed that we were getting duplicate validation messages because the attributes were being pulled off the models in multiple ways. I am not sure what the scenarios are where getting attributes from typedescriptors would come into play, but it was ugly having multiple of the same validation message. Thoughts?
| /// <param name="lifetime">The <see cref="ServiceLifetime"/> of the service.</param> | ||
| /// <typeparam name="TValidator">A class that implements <see cref="IValidate{T}"/> or <see cref="IAsyncValidate{T}"/></typeparam> | ||
| /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns> | ||
| public static IServiceCollection AddClassMiniValidator<TValidator>(this IServiceCollection services, ServiceLifetime lifetime = ServiceLifetime.Transient) |
There was a problem hiding this comment.
Not sure about this name, but this method is making it so that instead of this:
services.AddSingleton<IValidate<SomeModel>, SomeModelValidator>()
We can do this instead:
services.AddClassMiniValidator<SomeModelValidator>()
Adds support for defining
IValidate<T>classes to provide validation for class types outside of the model class where you are unable to alter the target model or would prefer to keep the model classes simple and not have theIValidatableObjectinterface implementation.